Skip to content

AOD: Disable MC tables for data - #13105

Merged
pzhristov merged 1 commit into
AliceO2Group:devfrom
f3sch:aod/mc_table
May 28, 2024
Merged

AOD: Disable MC tables for data#13105
pzhristov merged 1 commit into
AliceO2Group:devfrom
f3sch:aod/mc_table

Conversation

@f3sch

@f3sch f3sch commented May 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Now MC tables (even empty ones) are only written to disk when MC is active.
Marginally effects file size (reduction by a few KBs, e.g. tree headers).

Tested on local MC production generated with DOANALYSIS=1 ./O2DPG_pp_minbias.sh and with files from apass1_debug_build97 to reproduce data AO2D and run an analysis on this.

@f3sch

f3sch commented May 2, 2024

Copy link
Copy Markdown
Collaborator Author

Undrafting to start CI.

@f3sch
f3sch marked this pull request as ready for review May 2, 2024 10:53
@f3sch
f3sch requested a review from a team as a code owner May 2, 2024 10:53
@f3sch f3sch changed the title AOD: Disable MC tables for data AOD: Disable MC tables for data - DO NOT MERGE May 2, 2024
@f3sch
f3sch marked this pull request as draft May 2, 2024 19:41
@AliceO2Group AliceO2Group deleted a comment from github-actions Bot May 2, 2024
@AliceO2Group AliceO2Group deleted a comment from alibuild May 2, 2024
@f3sch
AOD: Disable MC tables for data
11c0f56 
Now MC tables (even empty ones) are only written to disk when MC is active.

Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
@f3sch
f3sch marked this pull request as ready for review May 2, 2024 20:41
@f3sch

f3sch commented May 5, 2024

Copy link
Copy Markdown
Collaborator Author

Hi, @jgrosseo, these changes would stop writing empty MC trees for data. I want to avoid breaking stuff downstream, do you have suggestion on what else I could test or anything that would immediately speak against doing this?

@jgrosseo

jgrosseo commented May 6, 2024

Copy link
Copy Markdown
Collaborator

Did you run sim_challenge.sh?

@jgrosseo

jgrosseo commented May 6, 2024

Copy link
Copy Markdown
Collaborator

For me it looks good from just checking the code. @ktf @aalkin Any comments?

@f3sch

f3sch commented May 6, 2024

Copy link
Copy Markdown
Collaborator Author

Yes, I ran 'sim_challenge.sh' successfully.

@aalkin

aalkin commented May 6, 2024

Copy link
Copy Markdown
Member

Other than "breaking" the current generalization, the code seems to do what is expected. I thinks this was tested long time ago and the effect on the AOD size was really marginal. Would it make more sense to actually provide the configuration to the AOD writer to store only certain subset of the tables created by AOD producer? It is more flexible.

@sawenzel

sawenzel commented May 6, 2024

Copy link
Copy Markdown
Collaborator

What are the benefits of doing this apart from a few bytes? I fear this might just add extra complication in the code without having a major impact on users. The data format specification of AOD tells that these tables are present. Empty data is probably easier to handle than missing tables.

@jgrosseo

jgrosseo commented May 6, 2024

Copy link
Copy Markdown
Collaborator

What are the benefits of doing this apart from a few bytes? I fear this might just add extra complication in the code without having a major impact on users. The data format specification of AOD tells that these tables are present. Empty data is probably easier to handle than missing tables.

Where does the specification tell you that for data you have MC tables? :)

@sawenzel

sawenzel commented May 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What are the benefits of doing this apart from a few bytes? I fear this might just add extra complication in the code without having a major impact on users. The data format specification of AOD tells that these tables are present. Empty data is probably easier to handle than missing tables.

Where does the specification tell you that for data you have MC tables? :)

Well, I thought you had a well-defined list of AOD tables written in the specification according to https://aliceo2group.github.io/analysis-framework/docs/datamodel/ao2dTables.html. If some of them are optional it is just fine with me. Maybe the doc would need to be more specific about this.

@jgrosseo

jgrosseo commented May 6, 2024

Copy link
Copy Markdown
Collaborator

What are the benefits of doing this apart from a few bytes? I fear this might just add extra complication in the code without having a major impact on users. The data format specification of AOD tells that these tables are present. Empty data is probably easier to handle than missing tables.

Where does the specification tell you that for data you have MC tables? :)

Well, I thought you had a well-defined list of AOD tables written in the specification. If some of them are optional it is just fine with me.

Yes, quite some are optional: E.g. Run 2 vs. Run 3. And for now MC tables have always been there but it was more a limitation that no one made the effort to make add the if statement in the aod producer.

Several times people got confused why our data has MC tables.

On the size-level it does not save much of course. An advantage of not having the tables is that the merger does not merge them (even if empty, it costs a few cycles in reco).

@f3sch f3sch changed the title AOD: Disable MC tables for data - DO NOT MERGE AOD: Disable MC tables for data May 14, 2024
@alibuild

alibuild commented May 20, 2024

Copy link
Copy Markdown
Collaborator

Error while checking build/O2/fullCI for 11c0f56 at 2024-05-21 00:36:

## sw/BUILD/O2-sim-challenge-test-latest/log
./sim-challenge.log[ERROR] Workflow crashed - PID 26301 (tof-matcher) did not exit correctly however it's not clear why. Exit code forced to 128.
./sim-challenge.log[ERROR]  - Device tof-matcher: pid 26301 (exit 128)
./sim-challenge.log[ERROR] SEVERE: Device tof-matcher (26301) returned with 128
./midreco.log[17873:MIDRecoWriter]: [22:33:56][ERROR] Some Lifetime::Timeframe data got dropped starting at 1
./digi.log[ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/37}
./digi.log[ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/38}
./digi.log[ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/40}
./digi.log[ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/42}
./digi.log[ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/43}
./digi.log[ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/45}
./digi.log[ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/46}
./digi.log[ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/47}
./digi.log[ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/48}
./digi.log[ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/49}
./digi.log[7621:internal-dpl-clock]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/37}
./digi.log[7621:internal-dpl-clock]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/38}
./digi.log[7621:internal-dpl-clock]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/40}
./digi.log[7621:internal-dpl-clock]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/42}
./digi.log[7621:internal-dpl-clock]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/43}
./digi.log[7621:internal-dpl-clock]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/45}
./digi.log[7621:internal-dpl-clock]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/46}
./digi.log[7621:internal-dpl-clock]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/47}
./digi.log[7621:internal-dpl-clock]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/48}
./digi.log[7621:internal-dpl-clock]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/49}
./digi.log[7626:SimReader]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/37}
./digi.log[7626:SimReader]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/38}
./digi.log[7626:SimReader]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/40}
./digi.log[7626:SimReader]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/42}
./digi.log[7626:SimReader]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/43}
./digi.log[7626:SimReader]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/45}
./digi.log[7626:SimReader]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/46}
./digi.log[7626:SimReader]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/47}
./digi.log[7626:SimReader]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/48}
./digi.log[7626:SimReader]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/49}
./digi.log[7631:internal-dpl-ccdb-backend]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/37}
./digi.log[7631:internal-dpl-ccdb-backend]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/38}
./digi.log[7631:internal-dpl-ccdb-backend]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/40}
./digi.log[7631:internal-dpl-ccdb-backend]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/42}
./digi.log[7631:internal-dpl-ccdb-backend]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/43}
./digi.log[7631:internal-dpl-ccdb-backend]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/45}
./digi.log[7631:internal-dpl-ccdb-backend]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/46}
./digi.log[7631:internal-dpl-ccdb-backend]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/47}
./digi.log[7631:internal-dpl-ccdb-backend]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/48}
./digi.log[7631:internal-dpl-ccdb-backend]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/49}
./digi.log[7636:TRDDigitizer]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/37}
./digi.log[7636:TRDDigitizer]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/38}
./digi.log[7636:TRDDigitizer]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/40}
./digi.log[7636:TRDDigitizer]: [ERROR] Found duplicate input binding with different spec.:collisioncontext {SIM/COLLISIONCONTEXT/42}
[0 more errors; see full log]

Full log here.

@pzhristov
pzhristov merged commit fdef300 into AliceO2Group:dev May 28, 2024
@f3sch
f3sch deleted the aod/mc_table branch November 25, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pzhristov pzhristov pzhristov approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@f3sch @jgrosseo @aalkin @sawenzel @alibuild @pzhristov